-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
> #3339 NPE caused by null defaultImage in XtextEditorTickUpdater (updateEditorImage) #3340
Conversation
can you also have a look at the subclasses |
There is only one subclass (XtendEditorErrorTickUpdater), which has only one field and that is not set to null.. |
Ah .. I've got it.. Classes which implement this interface IXtextEditorCallback :) |
3df46c9
to
e9d5e0f
Compare
Found one more case, where this could happen.. OverrideIndicatorModelListener uses the editor, which is unset and its not volatile.. I've also used it as local variable to avoid setting to null afterwards.. |
no i mean the xtend one. do we need the local stuff you do there too |
Oh now I looked again, the changes in the "OverrideIndicatorModelListener" are not necessary, because the field is accessed only in the UI thread.. So no worker thread should execute this code.. The problem happens only if the UI thread calls the beforeDispose, meanwhile a worker thread accesses the fields which are set by the beforeDispose method to null. If you check the stacktrace in the ticket, you will see that the defaultImage is accessed by the worker thread, where the UI thread already set this defaultImage to null.. |
We can keep the changes.. Maybe there is a worker thread which does the same for the next time.. So It shouldn't harm to keep these changes.. |
e9d5e0f
to
ba50210
Compare
I've done a rebase. |
@mehmet-karaman : could you please check what is wrong with your commit metadata (autor mail seem not match one expected by ECA)? |
ba50210
to
c37ac09
Compare
eclipsefdn/eca problem seems to be fixed. |
…ckUpdater (updateEditorImage)
c37ac09
to
fc5f46d
Compare
@cdietrich are we done here? |
i have no time to try |
I'm not very familiar with this part of the code base. |
Originally this bug was reported during our automated tests and from the stack trace it was almost obvious why that happened, since the test opens/closes Xtext editor and the background job simply runs after editor was already disposed. I haven't tried, but to reproduce one should create breakpoint at the job code ( IMHO the code was buggy and the fix is more or less trivial. |
I've recorded the behavior which was described by @iloveeclipse .. I hope it makes it clearer to understand the problem. (I could just save the video as 480p because 10mb was the maximum upload size for github, i hope its still readable) Videoprojekt.3.mp4The first breakpoint was hit in the worker thread (kept the thread suspended). After close of the editor the second breakpoint was hit, which sets the defaultImage to null (the main thread can be resumed), and after that the worker thread can be resumed and the NPE will be visible. |
@mehmet-karaman thanks for the detailed explanation. |
Set fields to volatile which could possible be accessed by multiple threads. Used local variables to store the value and added check with local variable.